Swift: implement type pruning for dataflow by rdmarsh2 · Pull Request #14592 · github/codeql

rdmarsh2 · 2023-10-25T20:19:38Z

This PR implements type pruning for dataflow by adding DataFlowType, which corresponds to only the canonical types in the Swift type hierarchy, adding a new getType member predicate to DataFlow::Node, and using them to implement the getNodeType predicate that type pruning relies on.

rdmarsh2 · 2023-10-25T20:20:29Z

Note that this currently includes some commits from #14570, which will hopefully be rebased away once that PR is merged.

geoffw0

Initial review. This looks good to me, though (inevitably on a PR of this size and complexity) I've made quite a lot of minor comments / questions. Testing will be important here - though I gather you've done quite a bit of debugging already. I'll have a quick go with the branch myself later today.

geoffw0 · 2023-10-26T08:22:05Z

 }

-predicate typeStrongerThan(DataFlowType t1, DataFlowType t2) { none() }
+predicate typeStrongerThan(DataFlowType t1, DataFlowType t2) {


What is this predicate for? Should it have QLDoc?

It's used in a few places in the dataflow library to select the more precise of two types, for instance when runtime overload resolution can tell us that a parameter has a more specific type than the argument that flows to it.

I requested QLDoc when it was introduced here: #13083 (comment), but it was postponed until we had turned the dataflow library into a shared qlpack. This has now been the case for a while so we should probably add QLDoc to this one now (cc @aschackmull)

Here you go: #14612

geoffw0 · 2023-10-26T08:25:44Z

-class DataFlowType extends TDataFlowType {
-  string toString() { result = "" }
+class DataFlowType extends Type {
+  DataFlowType() { this.getCanonicalType() = this }


What is the canonical type of a type exactly? (the QLDoc sadly reads "Gets the canonical type of this type.").

It's the unique type after resolving aliases and desugaring. For instance, if we have typealias MyInt == Int, then [MyInt?] has the canonical type Array<Optional<Int>>

I'm tempted to add getCanonicalType to Expr, Pattern, and whatever form of Decl makes sense, in the same way that we have getUnspecifiedType for C++

geoffw0 · 2023-10-26T08:27:11Z

+predicate compatibleTypes(DataFlowType t1, DataFlowType t2) {
+  exists(DataFlowType commonSub |
+    commonSub.getABaseType*().getCanonicalType() = stripType(t1) and
+    commonSub.getABaseType*().getCanonicalType() = stripType(t2)


I'm a bit worried about performance here, though probably the pragma[inline] keeps it relatively constrained. I guess DCA will confirm or refute my concerns...

geoffw0 · 2023-10-26T08:32:56Z

  }

-  cached
  private module Cached {


I'm a bit concerned about the name of this module if it's no longer cached.

Oops, I did that for debugging and didn't mean for it to land in the final commit.

geoffw0 · 2023-10-26T08:34:29Z

+  or
+  t1 instanceof AnyType
+  or
+  t2 instanceof AnyType


Do we need to account for stuff like AnyType? here?

geoffw0 · 2023-10-26T08:35:57Z

+    f1.getResult() = f2.getResult() and
+    not exists(int i | i in [0 .. f1.getNumberOfParamTypes()] |
+      f1.getParamType(i) != f2.getParamType(i)
+    ) and


I expected the = and != in here to be recursive calls to compatibleTypes. Should they be?

Ideally they would... Unfortunately doing that forces the relation to be materialized, which seems to be infeasible.

geoffw0 · 2023-10-26T08:37:24Z

@@ -1062,7 +1116,46 @@ string ppReprType(DataFlowType t) { none() }
 * a node of type `t1` to a node of type `t2`.


It might be worth stating that this relationship is not commutative, i.e. in some cases compatibleTypes(a, b) is not the same as compatibleTypes(b, a)? I found this surprising (but I think I understand why).

In the current implementation it's actually commutative. It isn't transitive, though.

I think the use of implies in the bit for a FunctionType makes it not commutative. compatibleTypes(notThrowingFunction, throwingFunction) might hold while compatibleTypes(throwingFunction, notThrowingFunction) does not.

compatibleTypes is actually required to be symmetric/commutative:

codeql/shared/dataflow/codeql/dataflow/DataFlow.qll

Line 111 in 04ca36f

* This predicate must be symmetric and reflexive.

. I do think that this relation is symmetric now, though, right?

geoffw0 · 2023-10-26T08:45:42Z

+    or
+    this.asExpr() instanceof InOutExpr
+    or
+    this.asExpr() instanceof InOutToPointerExpr


Possibly ForceValueExpr, OptionalSomePattern, AnyTryExpr, VarargExpansionExpr??? I'm really not sure though.

Yeah, all of those can change the type

geoffw0 · 2023-10-26T08:50:28Z

+    if this.hasSelfParam()
+    then result = this.getInterfaceType().(AnyFunctionType).getResult().(AnyFunctionType).getResult()
+    else result = this.getInterfaceType().(AnyFunctionType).getResult()
+  }


This is going to be widely useful. 👍

geoffw0 · 2023-10-26T08:52:30Z

@@ -0,0 +1,7 @@
+private import swift


I agree with the qldoc checks that this file and the AnyType class should have QLDoc. It can be short and to the point.

geoffw0 · 2023-10-26T14:58:43Z

+    ) and
+    (f1.isThrowing() implies f2.isThrowing()) and
+    (f1.isAsync() implies f2.isAsync())
+  )


I've just noticed t1, t2 are not bound to f1, f2 in this case.

oh jeez, that might be why it was blowing up...

Yeah, I'm seeing performance issues as well.

geoffw0

Sorry for taking a few days to get back to this. I've asked a couple of questions to aid my understanding.

We will need a new DCA run on this PR, when you think it's ready.

geoffw0 · 2023-11-10T10:27:38Z

+    not t instanceof AnyFunctionType
+  } or
+  TTopFunctionType() or
+  TDataFlowFunctionType(DataFlowCallable c)


What's the difference between TTopFunctionType and TDataFlowFunctionType? Why do we need both?

TTopFunctionType allows any function type, DataFlowFunctionType is a specific callable (and can be used to improve precision of lambda call resolution)

geoffw0 · 2023-11-10T10:35:04Z

+  )
+  or
+  rk.(ParamReturnKind).getIndex() = -1 and
+//  c.getSelfParam().isInout() and


Why did this line and 5 lines above get commented out?

geoffw0 · 2023-11-10T10:44:44Z

+  exists(t) and 
+  exists(pos) and
+  result.asType() instanceof AnyType
+}


What's going on here?

This predicate only applies to nodes that are synthesized from a callback in a modeled function. The AnyType is the top of Swift's type hierarchy (although it's not included in the getABaseType relationship), so we're allowing any type here. It's certainly possible to be more precise, though.

This looks like a cartesian product between all DataFlowTypes and all ArgumentPositions, right? This was fine before because DataFlowType was a unit type, but this looks like a real CP now, though? Or is the assumption that this fine because there aren't that many ArgumentPositions?

+  subType(t1) = subType(t2)
+  or
+  exists(BoundGenericType bound1, BoundGenericType bound2 |
+    stripType(t1.asType()).(BoundGenericType) = bound1 and


+  or
+  exists(BoundGenericType bound1, BoundGenericType bound2 |
+    stripType(t1.asType()).(BoundGenericType) = bound1 and
+    stripType(t2.asType()).(BoundGenericType) = bound2 and


+/*
+ * The Swift `Any` type, which is a supertype of all other types.
+ */


MathiasVP

Lots of comments (mostly just questions to help me understand the code), but I'm really happy that we're almost there 🎉

MathiasVP · 2023-12-14T16:05:39Z

+    result.asCallable().asSourceCallable() = expr
+    or
+    result.asCallable().asSourceCallable() = expr.(DeclRefExpr).getDecl()


Why do we need both cases here? Is that to handle each callable having their own type?

In any case, a comment on this would be helpful

That's handling lambdas vs references to functions

MathiasVP · 2023-12-14T16:11:37Z

  }
+
+  override DataFlowType getTypeImpl() {
+    result = getDataFlowType(this.asDefinition().getSourceVariable().asVarDecl().getType())


Not all SourceVariables has a result for asVarDecl (in particular, some of them are related to key paths: https://github.com/github/codeql/blob/main/swift/ql/lib/codeql/swift/dataflow/Ssa.qll#L28).

It seems like these don't have a type now? Should we instead have a getType predicate on SourceVariable (that all instances of the abstract class then implements) so that this predicate could instead be implemented as getDataFlowType(this.asDefinition().getSourceVariable())?

MathiasVP · 2023-12-14T16:16:42Z

+  override DataFlowType getTypeImpl() {
+    exists(BoundGenericType dictType, TupleType tupleType |
+      dictType = expr.getBase().getType().getCanonicalType() and
+      tupleType.getType(0) = dictType.getArgType(0) and
+      tupleType.getType(1) = dictType.getArgType(1) and
+      tupleType.getNumberOfTypes() = 2 and
+      result.asType() = tupleType
+      // TODO: handle generic parameter types
+    )
+  }


This looks very complicated 😨. Since DictionarySubscriptNode is really just an intermediate node that follows a subscript so that we can push both a TupleContent and a CollectionContent: could we instead simply get the type here by forwarding the call to the node representing the subscript expression?

I don't think so - the type of this node needs to be a tuple type so the read steps work out correctly.

MathiasVP · 2023-12-14T16:19:02Z

    KeyPathExpr getKeyPathExpr() { result = exit.getScope() }
+
+    override DataFlowType getTypeImpl() {
+      result = getDataFlowType(this.getKeyPathExpr().getType().(BoundGenericType).getArgType(1))


Why does this need to be getArgType(1)?

See the above comment.

MathiasVP · 2023-12-14T16:20:52Z


  KeyPathComponent getComponent() { result = component }
+
+  override DataFlowType getTypeImpl() { result = getDataFlowType(component.getComponentType()) }


Do we really need to implement getTypeImpl on all the post-update nodes? Can the result of getTypeImpl on a post-update node not simply be the result of calling getTypeImpl on the pre-update node?

Oh, I see what we do actually implement this predicate on PostUpdateNodes: https://github.com/github/codeql/pull/14592/files#diff-514dd0988f7b47e40ff8c1e66ea886d1f947d047da108270ed0ddec75c2856c1R1531. Do we really need this (and similar) overrides then?

Probably not. I can delete and recheck

That was actually only on ExprPostUpdateNodes. Moving it to PostUpdateNodeImpl simplifies some things, although it leads to a couple of duplicate overrides. It also fixed a bug with the postupdate nodes on keypaths.

MathiasVP · 2023-12-14T16:44:35Z

+  exists(TupleType tuple1, TupleType tuple2 |
+    stripType(t1.asType()) = tuple1 and
+    stripType(t2.asType()) = tuple2 and
+    compatibleTuples(tuple1, tuple2)


Similar question here: I think that compatibleTuples holds when there is a common index in which both tuple types is either the any type or an unbound type. When no such index exists, does that mean they're compatible because of the first condition in compatibleTypes?

MathiasVP · 2023-12-14T16:50:02Z

+  /**
+   * Gets the type of this node.
+   */
+  DataFlowType getType() { result = this.(NodeImpl).getTypeImpl() }


Do we really want to expose the DataFlowType type to users?

MathiasVP · 2023-12-14T16:53:38Z

+  exists(t) and 
+  exists(pos) and
+  result.asType() instanceof AnyType
+}


This looks like a cartesian product between all DataFlowTypes and all ArgumentPositions, right? This was fine before because DataFlowType was a unit type, but this looks like a real CP now, though? Or is the assumption that this fine because there aren't that many ArgumentPositions?

MathiasVP · 2023-12-14T16:55:17Z

+DataFlowType getCallbackReturnType(DataFlowType t, ReturnKind rk) {
+  exists(t) and
+  exists(rk) and
+  result.asType() instanceof AnyType
+}


Same here. This looks like a CP between all types and all return kinds. I guess this is fine if the getCallbackParameterType case is also fine?

It was fine but we don't actually need these anymore - they were part of the old API but not the new one.

MathiasVP · 2023-12-14T16:58:10Z

+  TupleType asUnlabeled() {
+    result.getNumberOfTypes() = this.getNumberOfTypes() and
+    not exists(int index | result.getType(index) != this.getType(index)) and
+    not exists(result.getName(_))
+  }


Two questions:

This looks like something that could blow up with a bad join order, right? That is, it we start by joining together all TupleTypes that has the same number of types. Does the joins look okay in this case?

Does the unlabeled version of a tuple type always exist? Or should the QLDoc say [...], if any.? And if this predicate really is partial: Is that a problem for the way we use it in stripType?

geoffw0 · 2023-12-15T16:23:22Z

Today is my last day before Christmas, so I'm probably not going to get chance to review the final version of this. I just want to post something to make it clear I do not expect you to wait for my final 👍. I trust @rdmarsh2 s work and @MathiasVP s judgement, so once you're both happy I'm happy. I do suggest you do another DCA run at some point though, and make sure any remaining regressions in performance or query results are at least understood.

geoffw0 · 2024-01-04T15:23:58Z

I think there are some comments above that haven't yet been addressed. Please would you reply to them, fix the failing checks, and state when this PR is ready to review again? Is there anything we can do to help?

rdmarsh2 requested a review from a team as a code owner October 25, 2023 20:19

github-actions Bot added Swift DataFlow Library labels Oct 25, 2023

geoffw0 reviewed Oct 26, 2023

View reviewed changes

github-actions Bot removed the DataFlow Library label Oct 26, 2023

geoffw0 mentioned this pull request Oct 27, 2023

Swift: QLDoc and test for getCanonicalType #14618

Merged

geoffw0 reviewed Nov 10, 2023

View reviewed changes

Robert Marsh and others added 18 commits December 13, 2023 19:40

Swift: WIP dataflow type pruning

6e3f79b

Swift: Type pruning WIP

4da8727

Swift: add getType to DataFlow::Node

f0528cd

Swift: fix Function.getResultType for generics

68d0ca7

Swift: fix parameter types in dataflow pruning

e6eaf97

Undo accidental removal of cached annotations

6d85063

Swift: model function types with a "top" type for performance

bc68e3f

Swift: introduce getDataFlowType

9a92b68

Swift: account for generic types in type pruning

ae0aebc

Swift: fix dictionary iterator flow with type pruning

7a4cfca

Swift: docs for AnyType

e45a33e

Swift: strip labels from tuples in type pruning

376a57a

Swift: fix a join order issue in compatibleTypes

a39fb48

Swift: uncomment accidentally commented-out lines

23866c5

Swift: autoformat

5b4152a

Swift: try to improve performance in compatibleTypes

eb55e97

Swift: more CompatibleTypes performance improvements

62cd974

Swift: Fix type pruning performance.

1ee6a03

rdmarsh2 force-pushed the rdmarsh2/swift/dataflow-type-pruning branch from 32ed674 to 1ee6a03 Compare December 13, 2023 19:40

github-advanced-security AI found potential problems Dec 13, 2023

View reviewed changes

MathiasVP reviewed Dec 14, 2023

View reviewed changes

Robert Marsh added 3 commits December 22, 2023 15:27

Merge branch 'main' into rdmarsh2/swift/dataflow-type-pruning

35573d2

Swift: fix missing type for keypath entry nodes

9c2bb77

Swift: switch to using preupdate node types for postupdate nodes

663c6ce

rdmarsh2 closed this Jan 16, 2024

		@@ -1062,7 +1116,46 @@ string ppReprType(DataFlowType t) { none() }
		* a node of type `t1` to a node of type `t2`.


		KeyPathComponent getComponent() { result = component }

		override DataFlowType getTypeImpl() { result = getDataFlowType(component.getComponentType()) }

Conversation

rdmarsh2 commented Oct 25, 2023

Uh oh!

rdmarsh2 commented Oct 25, 2023

Uh oh!

geoffw0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoffw0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdmarsh2 Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Check warning

Check warning

Check warning

MathiasVP left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

rdmarsh2 Nov 29, 2023 •

edited

Loading